KVStore: reduce log (#10813) by ti-chi-bot · Pull Request #10877 · pingcap/tiflash

ti-chi-bot · 2026-05-29T06:25:05Z

This is an automated cherry-pick of #10813

What problem does this PR solve?

Issue Number: ref #10814

Problem Summary:

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

Summary by CodeRabbit

Refactor
- Internal persistence logic refactored for safer message handling and more consistent flush/persist behavior.
- Debug/log messages enriched with additional context for flush and force-persist operations, improving observability.
- Safer string handling reduces risk of formatting issues during persistence.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2026-05-29T06:25:09Z

@JaySon-Huang This PR has conflicts, I have hold it.
Please resolve them or ask others to resolve them, then comment /unhold to remove the hold label.

ti-chi-bot · 2026-05-29T06:25:12Z

@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

coderabbitai · 2026-05-29T06:25:22Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 39316c6a-e618-4060-bccf-16440579ee04

📥 Commits

Reviewing files that changed from the base of the PR and between 7fc9959 and 99b9a65.

📒 Files selected for processing (1)

dbms/src/Storages/KVStore/MultiRaft/Persistence.cpp

🚧 Files skipped from review as they are similar to previous changes (1)

dbms/src/Storages/KVStore/MultiRaft/Persistence.cpp

📝 Walkthrough

Walkthrough

Method signatures for KVStore persistence were changed to use std::string_view; flush decision points now build structured diagnostic messages and pass them through forceFlushRegionDataImpl into persistRegion only when non-empty.

Changes

Persistence Messaging Modernization

Layer / File(s)	Summary
Header: Include and method signatures `dbms/src/Storages/KVStore/KVStore.h`	Adds `<string_view>` and updates signatures: `forceFlushRegionDataImpl` gains `std::string_view persist_extra_msg`; `persistRegion`'s `extra_msg` becomes `std::string_view`.
persistRegion implementation update `dbms/src/Storages/KVStore/MultiRaft/Persistence.cpp`	`persistRegion` accepts `extra_msg` as `std::string_view` and appends it into the caller string only when non-empty.
forceFlushRegionDataImpl signature & forwarding `dbms/src/Storages/KVStore/MultiRaft/Persistence.cpp`	`forceFlushRegionDataImpl` now accepts `persist_extra_msg` and forwards it to `persistRegion` instead of using an empty-string argument.
Message enrichment at flush call sites `dbms/src/Storages/KVStore/MultiRaft/Persistence.cpp`	`tryFlushRegionData` and `canFlushRegionDataImpl` build structured messages (force_persist_msg / flush_msg with term/index/truncated/gap/ids) and pass them into `forceFlushRegionDataImpl`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped through headers, found a view,
Replaced old chars with slices true,
Flushes whisper term and index bright,
Persist listens only when words alight,
A tidy trail of messages through the night.

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description largely follows the template structure but lacks substantive content; the commit-message block is empty, and the problem summary and 'What is changed' sections are not filled in, making it unclear what this PR actually accomplishes.	Fill in the Problem Summary and 'What is changed and how it works' sections with concrete details about the API refactoring, including rationale and impact. Include the commit message explaining the changes.
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'KVStore: reduce log (`#10813`)' is vague and does not clearly communicate the actual technical change—updating method signatures to use std::string_view for better logging context.	Replace with a more descriptive title that explains the actual change, such as 'KVStore: use std::string_view for extra_msg parameters in persist methods' or 'KVStore: refactor persistence logging to use string_view'.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@dbms/src/Storages/KVStore/MultiRaft/Persistence.cpp`:
- Around line 222-249: Resolve the leftover merge markers by keeping the
incoming flush_msg construction and the LOG_DEBUG that uses it, remove the
conflict markers and the old LOG_INFO block, and ensure flush_msg is declared
before the call to forceFlushRegionDataImpl so
forceFlushRegionDataImpl(curr_region, ..., flush_msg) compiles; also fix the
unknown accessor curr_region_ptr->getRegionTableSize() either by replacing it
with the correct existing accessor on the region object (e.g., the actual method
that returns the in-memory table size) or by adding/declaring
getRegionTableSize() in the region class, and keep usage of curr_region and
curr_region_ptr consistent.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 0246688f-e497-46db-8de6-7a4d16c7d8ad

📥 Commits

Reviewing files that changed from the base of the PR and between 2560b3d and 7fc9959.

📒 Files selected for processing (2)

dbms/src/Storages/KVStore/KVStore.h
dbms/src/Storages/KVStore/MultiRaft/Persistence.cpp

JaySon-Huang · 2026-05-29T06:46:55Z

/hold
wait for pingcap/tidb-engine-ext#457 and update proxy commit

Signed-off-by: JaySon-Huang <tshent@qq.com>

ti-chi-bot · 2026-05-29T09:15:12Z

[LGTM Timeline notifier]

Timeline:

2026-05-29 09:15:11.79680773 +0000 UTC m=+10652.171617446: ☑️ agreed by JinheLin.

ti-chi-bot · 2026-05-29T09:48:49Z

@kolafish: adding LGTM is restricted to approvers and reviewers in OWNERS files.

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

ti-chi-bot · 2026-05-29T09:48:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JinheLin, kolafish

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [JinheLin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

This is an automated cherry-pick of pingcap#10813

7fc9959

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request May 29, 2026

KVStore: reduce log #10813

Merged

12 tasks

ti-chi-bot Bot added the do-not-merge/cherry-pick-not-approved label May 29, 2026

ti-chi-bot assigned JaySon-Huang May 29, 2026

coderabbitai Bot reviewed May 29, 2026

View reviewed changes

Comment thread dbms/src/Storages/KVStore/MultiRaft/Persistence.cpp

try resolve conflict

99b9a65

Signed-off-by: JaySon-Huang <tshent@qq.com>

JinheLin approved these changes May 29, 2026

View reviewed changes

ti-chi-bot Bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label May 29, 2026

ti-chi-bot Bot added the approved label May 29, 2026

kolafish approved these changes May 29, 2026

View reviewed changes

ti-chi-bot Bot added cherry-pick-approved Cherry pick PR approved by release team. and removed do-not-merge/cherry-pick-not-approved labels May 29, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KVStore: reduce log (#10813)#10877

KVStore: reduce log (#10813)#10877
ti-chi-bot wants to merge 2 commits into
pingcap:release-8.5from
ti-chi-bot:cherry-pick-10813-to-release-8.5

ti-chi-bot commented May 29, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

ti-chi-bot commented May 29, 2026

Uh oh!

ti-chi-bot Bot commented May 29, 2026

Uh oh!

coderabbitai Bot commented May 29, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (2 warnings, 1 inconclusive)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

JaySon-Huang commented May 29, 2026

Uh oh!

ti-chi-bot Bot commented May 29, 2026

Uh oh!

ti-chi-bot Bot commented May 29, 2026

Uh oh!

ti-chi-bot Bot commented May 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

ti-chi-bot commented May 29, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Summary by CodeRabbit

Uh oh!

ti-chi-bot commented May 29, 2026

Uh oh!

ti-chi-bot Bot commented May 29, 2026

Uh oh!

coderabbitai Bot commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (2 warnings, 1 inconclusive)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JaySon-Huang commented May 29, 2026

Uh oh!

ti-chi-bot Bot commented May 29, 2026

[LGTM Timeline notifier]

Uh oh!

ti-chi-bot Bot commented May 29, 2026

Uh oh!

ti-chi-bot Bot commented May 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ti-chi-bot commented May 29, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 29, 2026 •

edited

Loading